-
Notifications
You must be signed in to change notification settings - Fork 255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PROPOSAL: Improve experience when retrying MultiPart request with Files #598
base: master
Are you sure you want to change the base?
Conversation
This is a common use case for ourselves (and we think for others as well) due to access token's short duration. As it expires, we automatically renew it with the refresh token, and finally do the original request again, that's why we fall into this problem. |
Thanks for the contribution. Please make sure all tests are passed. |
Thanks for the proposal. And It should be an issue if you put the file path in the request header? How about to add a extension function to provider a clone method to get a copy of FormData |
@trevorwang thanks for the review. My take on both points:
import 'package:dio/dio.dart';
extension FormDataExtension on FormData {
FormData clone() {
final formData = FormData();
for (final fieldEntry in fields) {
formData.fields.add(MapEntry(fieldEntry.key, fieldEntry.value));
}
for (final fileEntry in files) {
MultipartFile newMultipartFile = fileEntry.value;
final filePath = newMultipartFile.headers?['original_file_path']?.first;
if (filePath != null) {
newMultipartFile = MultipartFile.fromFileSync(
filePath,
filename: newMultipartFile.filename,
contentType: newMultipartFile.contentType,
headers: newMultipartFile.headers,
);
}
formData.files.add(MapEntry(fileEntry.key, newMultipartFile));
}
return formData;
}
} Also, I feel like I'll need some help understanding the tests. Is there any way I could have some of your time to guide me through it? Or could you guide me async? I've tried some (actually quite a lot) of things but they don't seem to work, even though the code itself is working. Sorry for bugging you with that :( |
@trevorwang tests should be working now :) let me know if you want me to include that |
Does it fix your issue with extension or the new header is still needed to be added? |
It alone won't fix the issue, please notice it uses the header in |
@trevorwang do you have an idea of what's going on with this failing test? I tried everything I could but I can't find a way to make it pass. I thought @kzrnm 's PR would solve it :( |
Sorry I misunderstood about above information. Extension is not necessary here. |
Thanks for pointing this out. I have tried generating from a list of files and it's gotten me an error I'm not sure how to handle, the generating method is different from the individual one. Here's what I did: 1- I set the keepFilePath as true in this test: @RestApi()
abstract class TestFileList {
@POST('/')
Future<void> testFileList(@Part(keepFilePath: true) List<File> files);
@POST('/')
Future<void> testOptionalFileList(@Part() List<File>? files);
@POST('/')
Future<void> testOptionalFile({@Part() File file});
} 2- I got this error and could not solve it, I believe the generation is not working:
3- I believe the error is somewhere here (lines 1857 to 1884), but could not correct it: else if (innerType != null &&
_typeChecker(File).isExactlyType(innerType)) {
final conType = contentType == null
? ''
: 'contentType: MediaType.parse(${literal(contentType)}),';
final keepFilePath = r.peek('keepFilePath')?.boolValue ?? false;
final filePath = refer(p.displayName).property('path');
final headers = keepFilePath
? literalMap({
'original_file_path': [filePath]
})
: '{}';
if (p.type.isNullable) {
blocks.add(Code('if (${p.displayName} != null) {'));
}
blocks.add(
refer(dataVar).property('files').property('addAll').call([
refer('''
${p.displayName}.map((i) => MapEntry(
'$fieldName',
MultipartFile.fromFileSync(i.path,
filename: i.path.split(Platform.pathSeparator).last,
$conType,
headers: $headers,
)))
''')
]).statement,
); Do you have any insight on what I could do here? |
You have to use
|
This should be a more ideal way to fix this reuse issue Future<void> _repeatedlyRequest() async {
Future<FormData> createFormData() async {
return FormData.fromMap({
'name': 'dio',
'date': DateTime.now().toIso8601String(),
'file': await MultipartFile.fromFile('./text.txt',filename: 'upload.txt'),
});
}
await dio.post('some-url', data: await createFormData());
} How about generate this kind of code for Multipart annotation? |
Hey @trevorwang, just fyi, I spent a lot of time trying to figure out the files list generation but was not able to make the tests pass. Do you mean generate a public method In my honest opinion, we could solve it only in the WDYT? Also, I believe I'm close to figuring out the texts and it's some synthax I don't yet quite understand. Let me know if you have 5 minutes to spare so that we could maybe figure this out synchronously. |
Hello there!
We at Revelo use Retrofit and its generator for all of our http requests. We also use MultiPart a lot and have gotten a significant amount of errors when retrying to send a MultiPart File because the stream has already been listened to.
exception: Bad state: The FormData has already been finalized. This typically means you are using the same FormData in repeated requests.. Error thrown .
I thought of a possible way to make retrying these requests possible and it involves including the [original file path] in the multipart file. At first I thought about inserting it in the file's headers, but I don't know if this could be a security breach or interfere in any other processes. And well, even though it would require us to rebuild the [files] field of the FormData, it's already better than having to implement another class just to deal with the paths of all of the files we're inserting and managing this in local storage.
To make this change, we would ideally add a new parameter in the Part() class, like [keepOriginalPath], which would be a bool. We could then use this bool in the generator to insert the path in the [headers] field's map when generating the code, if this field is given as true.
With this, we could retrieve the path in the RequestOptions object, and would not need to mess with Dio's structures as well.
WDYT? Would this ideia be ok? I was able to make it work properly but the tests got me confused, I would love some help from the team if I could have it and the idea makes sense to you.
Thanks a lot, best regards!